-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Run all miniphase transforms at group end #3350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run all miniphase transforms at group end #3350
Conversation
Seems not. Ycheck still passes the group where restoreScopes used to be run. |
81b61de
to
5afc086
Compare
The last phase block is not ychecked. This will mean that flatten and RestoreScopes are no longer checked. FTR ycheck used to find many bugs in those phases. |
If we suspect Flatten is wrong, we could always split it into its own group and Ycheck after it. But it won't run by default, needs manual intervention. |
test performance please |
performance test scheduled: 2 job(s) in queue, 1 running. |
@DarkDimius I think it is, LabelDefs is in the last group: https://github.com/lampepfl/dotty/blob/7c8f55741f7f85c26116357705b7db3e71739dfd/compiler/test/dotty/tools/vulpix/TestConfiguration.scala#L45 |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3350/ to see the changes. Benchmarks is based on merging with master (6d7b9c0) |
Only exception is RefChecks which produces errors when run at end of group. Probably because it clashes with ExtensionMethods.
Only exception: elimByName, which produces errors if run at group end.
LambdaLift cannot run at same phase as Flatten, so it's better to move Flatten one phase later. Some check files that dependent on the order in which symbols were accessed needed to be updated.
The other phase, LinkScala2Impls cannot yet be run at group end - it produces errors if one tries.
Instead of restoreScopes, use elimStaticThis, which is the new end of the group.
If all phases run at group end, any phase that adds or removes method parameters makes all phases run before it in the same group have the wrong number of parameters when type assigning an Apply or TypeApply. Previously this was allowed only if relaxedTyping was true. Now it is allowed everywhere. We should still be able to catch any errors in the transformer logic using Ycheck. With this change, LinkScala2Impls can now be run at group end.
Move to end of group. Needed a fix in ExtensionMethods to work correctly. Before the fix, extension methods got wrong flags.
We now have all miniphases running at group end. We can remove the infrastructure to enable other behavior. If some parts of some transformations need to run at a different phase than group end, we can always to `ctx.atPhase { ... }`.
It's no longer needed, since it's the default now.
Got deleted by accident before.
bcfafe1
to
3a0184c
Compare
TreeTransform used cpyBetweenPhases, but TreeTransformer did not. This does not seem sensical. Most likely this was overlooked. TreeTransform/TreeTransformer is too similar, one gets confused easily what is what.
Would be interesting to get some statistics on how much this reduces the number of Context objects we create. |
test performance please |
performance test scheduled: 2 job(s) in queue, 1 running. |
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3350/ to see the changes. Benchmarks is based on merging with master (e1365d9) |
|
||
/** Renames lifted classes to local numbering scheme */ | ||
class RenameLifted extends MiniPhaseTransform with SymTransformer { thisTransformer => | ||
|
||
override def phaseName = "renameLifted" | ||
|
||
override def runsAfterGroupsOf: Set[Class[_ <: Phases.Phase]] = Set(classOf[RestoreScopes]) | ||
// Not clear why this should run after restoreScopes | ||
// override def runsAfterGroupsOf: Set[Class[_ <: Phases.Phase]] = Set(classOf[RestoreScopes]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolasstucki Can you comment on that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally it was on Flatten
because this transformation acts on the flattened names. But @DarkDimius noted that the flattening transformation finishes in RestoreScopes
.
@@ -471,6 +464,15 @@ object TreeTransforms { | |||
|
|||
def miniPhases: Array[MiniPhase] | |||
|
|||
private var myGroupEndId: PhaseId = NoPhaseId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be private[this]
:)
@@ -404,7 +406,7 @@ trait TypeAssigner { | |||
} | |||
else { | |||
val argTypes = preCheckKinds(args, pt.paramInfos).tpes | |||
if (sameLength(argTypes, paramNames) || ctx.phase.prev.relaxedTyping) pt.instantiate(argTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it'd be nice to keep the relaxedTyping check when we can (so, at least in the Typer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll roll that into the next PR (#3366).
This avoids having to change the phase of the current context on each node visit. The performance tests indicate that this gives a performance improvement of 6-7% on dotty itself.